Fix #49: Add Scala.js support #109

OlivierBlanvillain · 2016-09-28T09:21:41Z

This PR adds Scala.js support.

The changes are pretty straightforward. After updating the bulid, a52ca9c moves files around to cope with the Scala.js jvm/js/shared folder structure. Almost everything goes to /shared, except for the few test cases that lead to linking errors on Scala.js. Then fc86cb2 copies the problematic tests to /js and manually comments the parts involving JVM specific stuff such as accesses to the file system.

I think the prime use case for xml literals in Scala.js is to generate equivalent org.scalajs.dom.raw.Node, which does not require any of the JVM specific parts of the library. Binding.scala and scala-js-react are already doing this by the mean of macros: they get ride of everything coming from scala.xml._ at compile time. This approach unfortunately leads to long compilation times. Using xml literals natively from Scala.js (probably with a run time transformation to org.scalajs.dom.raw.Node) would leads to much better compilation time.

biswanaths · 2016-09-28T09:40:18Z

Thanks @OlivierBlanvillain. I will take a look at the build failure https://travis-ci.org/scala/scala-xml/builds/163345736. And let the community take a look at this too.

biswanaths · 2016-09-29T13:27:20Z

Build seems fine https://travis-ci.org/scala/scala-xml/builds/163353923. There seems intermittent download error.

OlivierBlanvillain · 2016-09-29T13:50:28Z

Yes, I rolled back the version of sbt (I updated it without a good reason anyway), the problem seems to come from repo.typesafe.com (sbt/sbt#2758).

SethTisue · 2016-09-29T14:14:18Z

I rolled back the version of sbt (I updated it without a good reason anyway)

I'd encourage resubmitting that part once the sbt/sbt#2758 storm has passed

UPDATE (Oct 3): That storm has passed.

olafurpg · 2016-09-29T20:09:55Z

Wow, this would be a huge enabler for frontend Scala.js development. We are currently using Binding.scala in a medium sized project. Our designer loves using HTML syntax but he hates the abominable compilation times caused by the macro annotations that eliminate scala.xml._.

One question, it seems XML.load is commented out in all the js/ tests, presumably because it requires JVM specific APIs. Will there be a js alternative to Xml.load?

OlivierBlanvillain · 2016-09-30T12:31:17Z

@olafurpg Thanks for your feedback! XML.load relies on SAXParser which is JVM only. The best option would be to switch to a pure Scala XML parser, this fastparse implementation for example.

deontologic · 2016-10-03T15:53:47Z

I look forward to this being merged :)

biswanaths · 2016-10-13T14:05:06Z

What would be the right way to test this ? Any ideas ?

For a starter I will start with a basic scala.js app and try to use scala-xml and see how that pans out.

sjrd · 2016-10-13T14:32:08Z

What would be the right way to test this ?

Hum ... xmlJS/test?

biswanaths · 2016-10-13T14:47:34Z

@sjrd thank you for pitching in. Really appreciate your views regarding scalajs support.

I have gone through your post http://stackoverflow.com/questions/30143126/how-do-i-port-an-existing-scala-library-to-scalajs/30144349#30144349 and http://www.scala-js.org/doc/project/cross-build.html .

To be clearer, my question would have been is this sufficient or do we need to have any more tests, to make sure the library works and behaves well when used with scala.js.

sjrd · 2016-10-13T14:52:09Z

It is as sufficient as the tests for the JVM. Everything that should work on both platform should be tested in shared/src/test/. They will be tested with xmlJVM/test and xmlJS/test. I'm not sure what "more" you want. If you want more tests, you can add more tests, but that's just like on the JVM.

biswanaths · 2016-10-13T15:13:00Z

@sjrd thanks for quick reply and regarding tests I got it. I think we should be good enough to go on this then.

ashawley · 2016-10-13T15:35:41Z

build.sbt

+  .jsConfigure(_.enablePlugins(ScalaJSJUnitPlugin))
+
+lazy val xmlJVM = xml.jvm
+lazy val xmlJS = xml.js


The cleanup of the build.sbt file and modernization for other compilers is great.

Will the publish-signed task work with the new changes to SBT? The admin/build.sh file is run by Travis, and when it's for a Git tag, it will copy the admin/publish-settings.sbt file to the root directory. Looks like it will cause SBT to fail to load:

$ env PGP_PASSPHRASE=foobar sbt [info] Loading project definition from scala-xml/project scala-xml/publish-settings.sbt:3: error: not found: value pgpPassphrase pgpPassphrase := Some(env("PGP_PASSPHRASE").toArray) ^

ashawley · 2016-10-13T15:37:20Z

xml/js/src/test/scala/scala/xml/CompilerErrors.scala

@@ -0,0 +1,202 @@
+// package scala.xml


If there is no plan to fix the tests, maybe just don't copy them over? Just remove them from the js/src/test

FWIW, I agree.

ashawley · 2016-10-13T15:40:15Z

xml/js/src/test/scala/scala/xml/XMLTest.scala

+  //         Elem(null, "title", e, sc, Text("Foundations of Programming Languages"))),
+  //       Elem(null, "author", e, sc, Text("John Mitchell")),
+  //       Elem(null, "title", e, sc, Text("Foundations of Programming Languages"))))
+  // }


Again, if no plan to support these tests for the ScalaJS test suite, then just delete instead of comment out?

ashawley · 2016-10-13T15:42:22Z

xml/jvm/src/test/scala/scala/xml/ReuseNodesTest.scala

+ * uses as many old nodes as possible.
+ *
+ * Three transformers class for case -
+ * One for orginal, one for modified, and one proposed which shows
 * all are equivalent when it comes to reusing as many nodes as possible


If the only changes to this file, ReuseNodesTests.scala, are changes to whitespace, maybe revert so as to avoid touching the file besides the rename. We can fix whitespace in another PR

ashawley · 2016-10-13T15:43:06Z

xml/shared/src/main/scala/scala/xml/persistent/CachedFileStorage.scala

@@ -96,7 +97,7 @@ abstract class CachedFileStorage(private val file1: File) extends Thread {
    val c = fos.getChannel()

    // @todo: optimize
-    val storageNode = <nodes>{ nodes.toList }</nodes>
+    val storageNode = <nodes>{nodes.toList}</nodes>


This file, CachedFileStorage.scala, also only has whitespace changes that should be preserved for now, IMO

ashawley

I understand that scalajs needs the directory structure to be divided by jvm and js and shared. This is like the 3rd time the directory structure will change significantly for this long living package. This would be a good time to get it right, since the current structure frustrates me since it is overly long. This PR will make it longer. Sorry to attempt to tack these directory issues on to a PR for ScalaJS support, but so it goes.

Currently, it is

src/main/scala/scala/xml/
src/test/scala/scala/xml/

It would be better if it was this, IMO

src/main/scala/
src/test/scala/

For this ScalaJS PR, the current change renames things to

xml/js/src/test/scala/scala/xml/
xml/jvm/src/test/scala/scala/xml/
xml/shared/src/main/scala/scala/xml/
xml/shared/src/test/scala/scala/xml/

js/src/test/scala/
jvm/src/test/scala/
shared/src/main/scala/
shared/src/test/scala/

I don't have Scalajs experience, so I apologize for making suggestions out of ignorance.

OlivierBlanvillain · 2016-10-14T15:48:28Z

@ashawley thanks for you detailed review! I've forced push 3 updated commits which address all of your comment. Regarding the publish-signed did you forgot the cat admin/gpg.sbt >> project/plugins.sbt part?

For reference, I managed to sbt publish-signed this branche as "in.nvilla" %%% "scala-xml" % "1.0.6" using this build.sbt.

sjrd · 2016-10-14T15:55:34Z

It would be better if it was this, IMO
src/main/scala/
src/test/scala/

Whether good or bad, this would go against commonly acknowledged practices. Moreover, this would confuse some IDEs.

For the Scala.js thing, it might be better to achieve this:

js/src/test/scala/scala/xml/
jvm/src/test/scala/scala/xml/
shared/src/main/scala/scala/xml/
shared/src/test/scala/scala/xml/

which can be done with lazy val xml = crossProject.in(file(".")).... This removes the unnecessary xml/ prefix to all the paths.

ashawley · 2016-10-14T16:13:55Z

@OlivierBlanvillain Cool! Looks great, thanks. Yes, I missed that part of the build.sh script for publish-signed.

@sjrd I'm familiar with the convention, but wasn't aware the folder structure was a requirement for IDEs. I had assumed they use code indexing and such -- regardless of folder names to resolve things. I'm an SBT and Emacs scala-mode person, so if it's an issue I'd have to defer to others who use such IDEs.

OlivierBlanvillain · 2016-10-16T08:37:03Z

Alright, structure is now as follows:

js/src/test/scala/scala/xml/
jvm/src/test/scala/scala/xml/
shared/src/main/scala/scala/xml/
shared/src/test/scala/scala/xml/

OlivierBlanvillain · 2016-11-17T10:41:56Z

Rebased. Do you think this could get merged before 1.0.6?

olafurpg · 2016-11-17T10:46:14Z

FWIW, here is a demonstration of what's possible with scala-xml on Scala.js: https://olivierblanvillain.github.io/monadic-html/examples/

The demo uses a fork of scala-xml, it would be nice to depend on the official scala-xml module.

SethTisue · 2016-11-29T05:10:48Z

@biswanaths @ashawley shall we merge?

ashawley · 2016-11-29T13:52:50Z

LGTM. I appreciate @OlivierBlanvillain's willingness to minimize the file changes, and for entertaining my directory renaming wishes: Even if in the end, my whims were misguided.

I assume adding Scalacheck in #110 will require a similar pattern of disabling certain tests for ScalaJS?

sjrd · 2016-11-29T13:57:55Z

I assume adding Scalacheck in #110 will require a similar pattern of disabling certain tests for ScalaJS?

Correct.

biswanaths · 2016-11-30T18:08:10Z

@SethTisue , sure enough.

I think community had enough time to go through this.

I will go through the changes once again. And pull it in.

bbarker · 2017-01-08T14:54:36Z

Any other issues with this? Would really like to see it merged.

bbarker · 2017-02-04T13:16:23Z

@biswanaths @SethTisue @ashawley Nudge, anything I/we can do to help get this in?

I'd like to see it for scala.js, and once this is in, maybe experiment with using it in scala-native as well.

SethTisue · 2017-02-04T15:39:47Z

(the Scala team doesn't need to be the gatekeeper on everything in this repo, but this an especially juicy PR and there seems to be a consensus on merging it, and certainly plenty of review time passed, so I went ahead in this case)

ashawley · 2017-02-04T16:00:26Z

Travis build error for Java 8 and Scala 2.12:

unresolved dependency: org.scala-js#scalajs-junit-test-plugin_2.12.1;0.6.12: not found

SethTisue · 2017-02-04T16:03:50Z

huh, I don't know how I missed that, I thought I looked at the Travis results, maybe they were somehow not current. I'll prepare a followup PR right now with a fix.

this is required to fix Travis failures after scala#109 was merged

SethTisue · 2017-02-04T16:22:24Z

thank you, Olivier, for this truly substantial contribution!

ashawley · 2017-02-04T20:18:25Z

So will tests need to be added to both JS and JVM trees going forward? Was there an option to have SBT run common tests between both Scala compilers?

OlivierBlanvillain · 2017-02-04T20:34:46Z

@ashawley Cross platform tests should go in shared/src/test, jvm only tests in jvm/src/test. This PR introduced some duplication in tests which is a mistake. Everything in js/src/test should have been in shared/src/test, and jvm/src/test should only contains tests that are not in shared.

ashawley · 2017-02-04T20:37:45Z

That's right, shared. Ok, the duplication should be weeded out.

OlivierBlanvillain · 2017-02-04T20:41:04Z

@ashawley Let me open a new PR doing that

OlivierBlanvillain force-pushed the scala.js branch from fc86cb2 to 7ffd49a Compare September 28, 2016 10:00

ashawley reviewed Oct 13, 2016

View reviewed changes

ashawley requested changes Oct 13, 2016

View reviewed changes

OlivierBlanvillain force-pushed the scala.js branch from 7ffd49a to 419891f Compare October 14, 2016 14:57

OlivierBlanvillain force-pushed the scala.js branch from 419891f to 2e32f76 Compare October 16, 2016 08:35

OlivierBlanvillain force-pushed the scala.js branch from 2e32f76 to bbf0a23 Compare October 16, 2016 08:39

OlivierBlanvillain force-pushed the scala.js branch from bbf0a23 to c2418f4 Compare November 17, 2016 09:21

OlivierBlanvillain mentioned this pull request Nov 19, 2016

Scala 2.12 version? OlivierBlanvillain/monadic-html#24

Closed

OlivierBlanvillain added 3 commits December 1, 2016 10:20

Prepare build.sbt for Scala.js support

abdb3eb

Move /src → /jvm/src, split test between /jvm and /shared

d065e11

Dupliate Scala.js linkable tests from jvm/test to js/test

834bfab

OlivierBlanvillain force-pushed the scala.js branch from c2418f4 to 834bfab Compare December 1, 2016 09:22

SethTisue merged commit 00fa469 into scala:master Feb 4, 2017

SethTisue added a commit to SethTisue/scala-xml that referenced this pull request Feb 4, 2017

use latest Scala.js

6e58d6a

this is required to fix Travis failures after scala#109 was merged

SethTisue mentioned this pull request Feb 4, 2017

use latest Scala.js #131

Merged

ashawley mentioned this pull request Feb 4, 2017

Add ScalaCheck specs #110

Closed

OlivierBlanvillain mentioned this pull request Feb 4, 2017

Remove test duplication #133

Merged

OlivierBlanvillain deleted the scala.js branch February 4, 2017 20:54

bbarker mentioned this pull request Mar 26, 2017

Can a scala-js version of this be published? #49

Closed

ashawley mentioned this pull request Oct 9, 2017

Publish release as 1.1.0 #156

Closed

3 tasks

bbarker mentioned this pull request Feb 25, 2018

Compile CBT itself with Scala Native cvogt/cbt#129

Open

SethTisue mentioned this pull request Mar 11, 2018

Scala.js cannot load XML #198

Open

Fix #49: Add Scala.js support #109

Fix #49: Add Scala.js support #109

Conversation

OlivierBlanvillain commented Sep 28, 2016

biswanaths commented Sep 28, 2016

biswanaths commented Sep 29, 2016 • edited Loading

OlivierBlanvillain commented Sep 29, 2016

SethTisue commented Sep 29, 2016 • edited Loading

olafurpg commented Sep 29, 2016

OlivierBlanvillain commented Sep 30, 2016 • edited Loading

deontologic commented Oct 3, 2016

biswanaths commented Oct 13, 2016 • edited Loading

sjrd commented Oct 13, 2016

biswanaths commented Oct 13, 2016 • edited Loading

sjrd commented Oct 13, 2016

biswanaths commented Oct 13, 2016

ashawley Oct 13, 2016

Choose a reason for hiding this comment

ashawley Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

ashawley Oct 13, 2016

Choose a reason for hiding this comment

sjrd Oct 13, 2016

Choose a reason for hiding this comment

ashawley Oct 13, 2016

Choose a reason for hiding this comment

ashawley Oct 13, 2016

Choose a reason for hiding this comment

ashawley Oct 13, 2016

Choose a reason for hiding this comment

ashawley left a comment • edited Loading

Choose a reason for hiding this comment

OlivierBlanvillain commented Oct 14, 2016

sjrd commented Oct 14, 2016 • edited Loading

ashawley commented Oct 14, 2016 • edited Loading

OlivierBlanvillain commented Oct 16, 2016

OlivierBlanvillain commented Nov 17, 2016

olafurpg commented Nov 17, 2016

SethTisue commented Nov 29, 2016

ashawley commented Nov 29, 2016

sjrd commented Nov 29, 2016

biswanaths commented Nov 30, 2016

bbarker commented Jan 8, 2017

bbarker commented Feb 4, 2017

SethTisue commented Feb 4, 2017 • edited Loading

ashawley commented Feb 4, 2017

SethTisue commented Feb 4, 2017

SethTisue commented Feb 4, 2017

ashawley commented Feb 4, 2017

OlivierBlanvillain commented Feb 4, 2017

ashawley commented Feb 4, 2017

OlivierBlanvillain commented Feb 4, 2017 • edited Loading

biswanaths commented Sep 29, 2016 •

edited

Loading

SethTisue commented Sep 29, 2016 •

edited

Loading

OlivierBlanvillain commented Sep 30, 2016 •

edited

Loading

biswanaths commented Oct 13, 2016 •

edited

Loading

biswanaths commented Oct 13, 2016 •

edited

Loading

ashawley Oct 13, 2016 •

edited

Loading

ashawley left a comment •

edited

Loading

sjrd commented Oct 14, 2016 •

edited

Loading

ashawley commented Oct 14, 2016 •

edited

Loading

SethTisue commented Feb 4, 2017 •

edited

Loading

OlivierBlanvillain commented Feb 4, 2017 •

edited

Loading